fix: center and properly size event banners regardless of aspect ratio (#746) - #752
fix: center and properly size event banners regardless of aspect ratio (#746)#752Akashstztt wants to merge 2 commits into
Conversation
Project-HAMi#746) Signed-off-by: Akash Kumar <akashstzt@gmail.com>
✅ Deploy Preview for project-hami ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Akashstztt The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe event banner image is wrapped in a centered container. Its styles now preserve intrinsic sizing, use ChangesEvent banner layout
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/EventLanding.module.css`:
- Around line 18-23: Update the .banner styling so max-height: 480px applies
only to square and portrait banners, while landscape banners can scale to the
available wrapper width without the global height cap. Preserve intrinsic
aspect-ratio behavior and the existing max-width constraint.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 295087f8-3f4b-4f04-8648-36dce6bb989a
📒 Files selected for processing (2)
src/components/EventLanding.jssrc/components/EventLanding.module.css
…ead of conflicting width/height auto Signed-off-by: Akash Kumar <akashstzt@gmail.com>
|
@mesutoezdil @wawa0210 @rootsongjc — this should now be ready for review. I fixed the CodeRabbit comment about the height cap on landscape banners by using a fixed-size wrapper approach. Let me know if you have any concerns! |
1 participant
Related to #746
What type of PR is this?
/kind bug
What this PR does / why we need it:
The event banner on
/landing/kcd-vietnamwas being cropped because.bannerusedobject-fit: cover, which is designed for wide/landscape images. This event's banner is square, so key visual content (title, speaker names) was getting cut off.Fixed by wrapping the banner image in a flex-centered container and using
max-width/max-heightwithobject-fit: containandwidth/height: auto, so the image sizes itself based on its actual aspect ratio instead of being forced to stretch full-width. This displays square banners fully and centered, while landscape banners (e.g./landing/kubecon-japan) still fill the width naturally.Which issue(s) this PR fixes:
Fixes #746
Checklist:
npm run lintandnpm run format:checkpass — note: both report pre-existing repo-wide issues unrelated to this PR (a Windows markdownlint CLI quirk, and 482 files with existing Prettier formatting drift). Neither of the two files changed in this PR is affected.npm run buildsucceeds for bothenandzh— not verified locally due to a Windows-specificNODE_OPTIONSshell syntax issue in this environment; CI will validate this on Linux.git commit -s)Testing:
/landing/kcd-vietnambanner (square image) now displays fully, centered, with no cropping or letterboxing./landing/kubecon-japanbanner (landscape image) still displays correctly with no regression.EventLanding.module.cssis only imported byEventLanding.js, which is only used by these two event pages — no other pages affected by this change.